Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix interpolation #30

Merged
merged 10 commits into from
Jan 29, 2024
Merged

Fix interpolation #30

merged 10 commits into from
Jan 29, 2024

Conversation

PerezHz
Copy link
Owner

@PerezHz PerezHz commented Jan 25, 2024

No description provided.

@PerezHz PerezHz marked this pull request as ready for review January 29, 2024 19:30
@PerezHz PerezHz changed the title WIP: Fix interpolation Fix interpolation Jan 29, 2024
@PerezHz
Copy link
Owner Author

PerezHz commented Jan 29, 2024

@lbenet @LuEdRaMo these changes fix most of the problems with @code_warntype for the evaluation of TaylorInterpolants, while being backwards-compatible (i.e., no re-issuing of the ephemeris is needed). I tested this PR with @LuEdRaMo's PerezHz/NEOs.jl#31 and everything is looking fine, but let me know if there are any issues. There is only one case where I could not get rid of the ::Anys, but that seems to be an issue with TaylorSeries (?).

Another thing to note is that there are some on TaylorInterpolant methods defined in NEOs.jl, like zero and some serializations. I think we should bring those to the same place (i.e., here), but we can leave that for another PR. Lastly, I'm just thinking out loud here, but the functionality of the TaylorInterpolant struct can be argued to be more general than only PE and NEOs.jl, so probably it makes sense to eventually put it in TaylorIntegration.

Since tests are passing, I'll bump the minor version and go ahead and merge this.

@PerezHz
Copy link
Owner Author

PerezHz commented Jan 29, 2024

'll bump the minor version

(I meant the patch version)

@PerezHz
Copy link
Owner Author

PerezHz commented Jan 29, 2024

There is only one case where I could not get rid of the ::Anys, but that seems to be an issue with TaylorSeries (?).

Regarding this, I'll investigate further and report an issue in TaylorSeries if needed.

@PerezHz PerezHz merged commit 17f8f5b into main Jan 29, 2024
3 checks passed
@PerezHz PerezHz deleted the jp/interpolation-fix branch January 29, 2024 20:27
@lbenet
Copy link
Collaborator

lbenet commented Jan 29, 2024

There is only one case where I could not get rid of the ::Anys, but that seems to be an issue with TaylorSeries (?).

Can you provide some details? Maybe I can help 😄 . One more question: have you tested this PR using lb/improve_mixtures2 branch?

@PerezHz
Copy link
Owner Author

PerezHz commented Jan 29, 2024

Can you provide some details? Maybe I can help 😄 . One more question: have you tested this PR using lb/improve_mixtures2 branch?

Of course, many thanks! On current master I get:

julia> using TaylorSeries

julia> x = Taylor1.([rand(3) for _ in 1:5, __ in 1:2])
5×2 Matrix{Taylor1{Float64}}:
   0.8440031850115604 + 0.27396498091154997 t + 0.8593268973294502+ 𝒪(t³)         0.304833144771666 + 0.8015270517578177 t + 0.6663648827023969+ 𝒪(t³)
   0.4769667257928858 + 0.18328727716869164 t + 0.7720343081783517+ 𝒪(t³)          0.25023114334238694 + 0.5608979582062102 t + 0.65675485312088+ 𝒪(t³)
   0.3558175964225522 + 0.34934157610451766 t + 0.8965424895085355+ 𝒪(t³)      0.06192817828628172 + 0.03617621509228641 t + 0.27532384700131196+ 𝒪(t³)
  0.3761780646204895 + 0.006473829252253704 t + 0.6103337009204509+ 𝒪(t³)       0.28422146538051607 + 0.47690611728020404 t + 0.6515769217433143+ 𝒪(t³)
    0.9324978508223983 + 0.4249540247058746 t + 0.6168014064890693+ 𝒪(t³)         0.9948579177745833 + 0.4566885347215339 t + 0.6809588553613743+ 𝒪(t³)

julia> dq = TaylorSeries.set_variables("dq", order=2, numvars=2)
2-element Vector{TaylorN{Float64}}:
  1.0 dq₁ + 𝒪(‖x‖³)
  1.0 dq₂ + 𝒪(‖x‖³)

julia> x(3.04 + dq[1])
5×2 Matrix{TaylorN{Float64}}:
    9.61841218134252 + 5.498672516674608 dq₁ + 0.8593268973294502 dq₁² + 𝒪(‖x‖³)        8.899753082097902 + 4.853025538588391 dq₁ + 0.6663648827023969 dq₁² + 𝒪(‖x‖³)
    8.168992310846765 + 4.87725587089307 dq₁ + 0.7720343081783517 dq₁² + 𝒪(‖x‖³)            8.02482658689119 + 4.553967465181161 dq₁ + 0.65675485312088 dq₁² + 𝒪(‖x‖³)
  9.703303058822367 + 5.8003199123164135 dq₁ + 0.8965424895085355 dq₁² + 𝒪(‖x‖³)      2.7163367366141573 + 1.7101452048602632 dq₁ + 0.27532384700131196 dq₁² + 𝒪(‖x‖³)
   6.03631843597378 + 3.7173027308485955 dq₁ + 0.6103337009204509 dq₁² + 𝒪(‖x‖³)         7.755629341895349 + 4.438493801479555 dq₁ + 0.6515769217433143 dq₁² + 𝒪(‖x‖³)
    7.92458996413764 + 4.175106576159417 dq₁ + 0.6168014064890693 dq₁² + 𝒪(‖x‖³)          8.676340421035723 + 4.59691837531869 dq₁ + 0.6809588553613743 dq₁² + 𝒪(‖x‖³)

julia> @code_warntype x(3.04 + dq[1])
MethodInstance for (::Matrix{Taylor1{Float64}})(::TaylorN{Float64})
  from (p::AbstractArray{Taylor1{T}})(x) where T<:Number @ TaylorSeries ~/.julia/dev/TaylorSeries/src/evaluate.jl:106
Static Parameters
  T = Float64
Arguments
  p::Matrix{Taylor1{Float64}}
  x::TaylorN{Float64}
Body::Any
1%1 = Base.broadcasted(TaylorSeries.evaluate, p, x)::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{2}, Nothing, typeof(evaluate), Tuple{Matrix{Taylor1{Float64}}, TaylorN{Float64}}}
│   %2 = Base.materialize(%1)::Any
└──      return %2

(note the ::Anys near the end). If I run the same code on lb/improve_mixtures2 I still get the same result (if I did things right)...

@lbenet
Copy link
Collaborator

lbenet commented Jan 29, 2024

Current master refers to TaylorSeries, I guess...

@lbenet
Copy link
Collaborator

lbenet commented Jan 29, 2024

In the branch lb/improve_mixtures2 I get the same. We should probably continue this discussion in JuliaDiff/TaylorSeries.jl#333 ...

@PerezHz
Copy link
Owner Author

PerezHz commented Jan 29, 2024

Current master refers to TaylorSeries, I guess...

Indeed, I meant TaylorSeries

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants